Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[POWEROPS-2558] chore: linters and codecov in workflows #406

Merged
merged 7 commits into from
Aug 8, 2024

Conversation

nodegard
Copy link
Contributor

@nodegard nodegard commented Aug 7, 2024

Description

Please describe the change you have made.

Checklist:

Copy link

codecov bot commented Aug 7, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@nodegard nodegard changed the title [POWEROPS-2558] chore: linters [POWEROPS-2558] chore: linters and codecov in workflows Aug 7, 2024
@nodegard nodegard marked this pull request as ready for review August 7, 2024 20:07
@nodegard nodegard requested a review from a team as a code owner August 7, 2024 20:07
Copy link
Contributor

@eriklien eriklien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just added a few questions for my understanding.

Comment on lines +59 to +62
SETTINGS__COGNITE__CLIENT_ID: ${{ vars.CLIENT_ID }}
SETTINGS__COGNITE__PROJECT: ${{ vars.PROJECT }}
SETTINGS__COGNITE__CDF_CLUSTER: ${{ vars.CLUSTER }}
SETTINGS__COGNITE__TENANT_ID: ${{ vars.TENANT_ID }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Is this pointing to environment variables in the CI environment? I don't see them there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're on the repo level because all the envs need them

Comment on lines -11 to -30
lint:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: "3.9"

# The installation is required as mypy must be run in the local system environment, not in the pre-commit environment.
- name: Install required dependencies
run: |
python3 -m pip install --upgrade pip poetry
poetry config virtualenvs.create false
poetry install

- name: Linting and static code checks
run: |
pre-commit run --all-files

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Not needed here because we do it in build.yml?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, no need to re-run pre-commit on merge to main given the branch protections and strict requirements for passing on PRs toward main

@nodegard nodegard merged commit 2019d56 into main Aug 8, 2024
7 checks passed
@nodegard nodegard deleted the chore/linters branch August 8, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants